Automated Test: notification-rule-enhancements #336
Closed
+542
−418
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* wip * Add working actions for GMA rules based on Prom-only API * Remove Ruler-loader related code for Grafana rules Co-authored-by: Sonia Augilar <sonia.aguilar@grafana.com> * Remove outdated tests * add some comments * remove commented code * remove showLocation property * Add missing mocks in tests * Add showLocation to GrafanaRuleListItem, improve useAbilities, address PR feedback * Enhance GrafanaGroupLoader tests: Add permission checks and More button functionality - Introduced user permission grants for alerting actions in tests. - Added tests for rendering the More button with action menu options. - Verified that each rule has its own action buttons and handles permissions correctly. - Ensured the edit button is not rendered when user lacks edit permissions. - Confirmed the correct menu actions are displayed when the More button is clicked. * Update translations --------- Co-authored-by: Sonia Aguilar <soniaaguilarpeiron@gmail.com> Co-authored-by: Sonia Augilar <sonia.aguilar@grafana.com>
📝 WalkthroughWalkthroughThis PR introduces Grafana Prometheus rule support to the alerting system by adding grouped ability/permission hooks, refactoring components to use batched permission checks, and simplifying Grafana rule list loading to rely exclusively on Prometheus data. The changes add new hooks for querying multiple Grafana rule permissions, update existing components to use these hooks, and introduce a new GrafanaRuleListItem component for rule rendering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@public/app/features/alerting/unified/hooks/useAbilities.ts`:
- Around line 287-334: The hook useAllGrafanaPromRuleAbilities must return
NotSupported for all actions when rule is undefined (skipToken); inside the
useMemo in useAllGrafanaPromRuleAbilities, add an early guard like if (!rule) {
return an Abilities<AlertRuleAction> object where every AlertRuleAction maps to
NotSupported } so
MaybeSupported/MaybeSupportedUnlessImmutable/duplicateSupported are not allowed
to default to AlwaysSupported and accidentally enable actions for non-Grafana
rules; update the returned abilities to use NotSupported for Duplicate, View,
Update, Delete, Explore, Silence, ModifyExport, Pause, Restore,
DeletePermanently when rule is missing.
In `@public/app/features/alerting/unified/rule-list/FilterView.tsx`:
- Around line 157-163: The GrafanaRuleListItem rendered inside the .map() loop
is missing a stable key prop which can cause React list rendering issues; update
the GrafanaRuleListItem JSX to include key={key}, matching the other branches
(DataSourceRuleLoader and UnknownRuleListItem) that already use the key variable
from the map destructuring so each item has a stable identifier.
🧹 Nitpick comments (2)
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx (1)
36-43: Unuseddeletemenu item selector.The
ui.menuItems.delete()selector is defined here but never used in any test assertions. Meanwhile, the test at line 207 expects exactly 4 menu items, which aligns with the 4 items actually asserted (silence, copyLink, duplicate, export).Either:
- Remove the unused
deleteselector if the delete action isn't expected in this menu- Add test coverage for the delete action if it should appear when
AlertingRuleDeletepermission is grantedpublic/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (1)
24-28: Stale documentation comment.The JSDoc comment states "Loads an evaluation group from Prometheus and Ruler endpoints" but the implementation now only queries Prometheus. The Ruler-based matching logic has been removed per the AI summary.
📝 Suggested documentation update
/** - * Loads an evaluation group from Prometheus and Ruler endpoints. + * Loads an evaluation group from the Prometheus endpoint. * Displays a loading skeleton while the data is being fetched. * Polls the Prometheus endpoint every 20 seconds to refresh the data. */
Comment on lines
+287
to
+334
| export function useAllGrafanaPromRuleAbilities(rule: GrafanaPromRuleDTO | undefined): Abilities<AlertRuleAction> { | ||
| // For GrafanaPromRuleDTO, we use useIsGrafanaPromRuleEditable instead | ||
| const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); // duplicate | ||
| const [_, exportAllowed] = useAlertingAbility(AlertingAction.ExportGrafanaManagedRules); | ||
| const canSilence = useCanSilence(rule); | ||
|
|
||
| const silenceSupported = useGrafanaRulesSilenceSupport(); | ||
| const canSilenceInFolder = useCanSilenceInFolder(rule?.folderUid); | ||
|
|
||
| const abilities = useMemo<Abilities<AlertRuleAction>>(() => { | ||
| const isProvisioned = rulerRuleType.grafana.rule(rule) && Boolean(rule.grafana_alert.provenance); | ||
| // const isFederated = isFederatedRuleGroup(); | ||
| const isProvisioned = rule ? isProvisionedPromRule(rule) : false; | ||
|
|
||
| // Note: Grafana managed rules can't be federated - this is strictly a Mimir feature | ||
| // See: https://grafana.com/docs/mimir/latest/references/architecture/components/ruler/#federated-rule-groups | ||
| const isFederated = false; | ||
| const isGrafanaManagedAlertRule = rulerRuleType.grafana.rule(rule); | ||
| // All GrafanaPromRuleDTO rules are Grafana-managed by definition | ||
| const isAlertingRule = prometheusRuleType.grafana.alertingRule(rule); | ||
| const isPluginProvided = isPluginProvidedRule(rule); | ||
|
|
||
| // if a rule is either provisioned, federated or provided by a plugin rule, we don't allow it to be removed or edited | ||
| const immutableRule = isProvisioned || isFederated || isPluginProvided; | ||
|
|
||
| // while we gather info, pretend it's not supported | ||
| const MaybeSupported = loading ? NotSupported : isRulerAvailable; | ||
| // GrafanaPromRuleDTO rules are always supported (no loading state for ruler availability) | ||
| const MaybeSupported = loading ? NotSupported : AlwaysSupported; | ||
| const MaybeSupportedUnlessImmutable = immutableRule ? NotSupported : MaybeSupported; | ||
|
|
||
| // Creating duplicates of plugin-provided rules does not seem to make a lot of sense | ||
| const duplicateSupported = isPluginProvided ? NotSupported : MaybeSupported; | ||
|
|
||
| const rulesPermissions = getRulesPermissions(rulesSourceName); | ||
| const rulesPermissions = getRulesPermissions('grafana'); | ||
|
|
||
| const abilities: Abilities<AlertRuleAction> = { | ||
| [AlertRuleAction.Duplicate]: toAbility(duplicateSupported, rulesPermissions.create), | ||
| [AlertRuleAction.View]: toAbility(AlwaysSupported, rulesPermissions.read), | ||
| [AlertRuleAction.Update]: [MaybeSupportedUnlessImmutable, isEditable ?? false], | ||
| [AlertRuleAction.Delete]: [MaybeSupportedUnlessImmutable, isRemovable ?? false], | ||
| [AlertRuleAction.Explore]: toAbility(AlwaysSupported, AccessControlAction.DataSourcesExplore), | ||
| [AlertRuleAction.Silence]: canSilence, | ||
| [AlertRuleAction.ModifyExport]: [isGrafanaManagedAlertRule, exportAllowed], | ||
| [AlertRuleAction.Pause]: [MaybeSupportedUnlessImmutable && isGrafanaManagedAlertRule, isEditable ?? false], | ||
| [AlertRuleAction.Restore]: [MaybeSupportedUnlessImmutable && isGrafanaManagedAlertRule, isEditable ?? false], | ||
| [AlertRuleAction.Silence]: [silenceSupported, canSilenceInFolder && isAlertingRule], | ||
| [AlertRuleAction.ModifyExport]: [isAlertingRule, exportAllowed], | ||
| [AlertRuleAction.Pause]: [MaybeSupportedUnlessImmutable && isAlertingRule, isEditable ?? false], | ||
| [AlertRuleAction.Restore]: [MaybeSupportedUnlessImmutable && isAlertingRule, isEditable ?? false], | ||
| [AlertRuleAction.DeletePermanently]: [ | ||
| MaybeSupportedUnlessImmutable && isGrafanaManagedAlertRule, | ||
| MaybeSupportedUnlessImmutable && isAlertingRule, | ||
| (isRemovable && isAdmin()) ?? false, | ||
| ], | ||
| }; | ||
|
|
||
| return abilities; | ||
| }, [rule, loading, isRulerAvailable, rulesSourceName, isEditable, isRemovable, canSilence, exportAllowed]); | ||
| }, [rule, loading, isEditable, isRemovable, canSilenceInFolder, exportAllowed, silenceSupported]); |
There was a problem hiding this comment.
Ensure skipToken results in “not supported” abilities.
When skipToken is used, rule becomes undefined, but MaybeSupported currently defaults to AlwaysSupported, which can enable actions (e.g., Duplicate) for non‑Grafana rules. Consider short‑circuiting to all‑NotSupported when rule is undefined.
🛠️ Suggested guard
export function useAllGrafanaPromRuleAbilities(rule: GrafanaPromRuleDTO | undefined): Abilities<AlertRuleAction> {
// For GrafanaPromRuleDTO, we use useIsGrafanaPromRuleEditable instead
const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); // duplicate
const [_, exportAllowed] = useAlertingAbility(AlertingAction.ExportGrafanaManagedRules);
const silenceSupported = useGrafanaRulesSilenceSupport();
const canSilenceInFolder = useCanSilenceInFolder(rule?.folderUid);
+
+ if (!rule) {
+ return {
+ [AlertRuleAction.Duplicate]: [NotSupported, false],
+ [AlertRuleAction.View]: [NotSupported, false],
+ [AlertRuleAction.Update]: [NotSupported, false],
+ [AlertRuleAction.Delete]: [NotSupported, false],
+ [AlertRuleAction.Explore]: [NotSupported, false],
+ [AlertRuleAction.Silence]: [NotSupported, false],
+ [AlertRuleAction.ModifyExport]: [NotSupported, false],
+ [AlertRuleAction.Pause]: [NotSupported, false],
+ [AlertRuleAction.Restore]: [NotSupported, false],
+ [AlertRuleAction.DeletePermanently]: [NotSupported, false],
+ };
+ }Also applies to: 380-405
🤖 Prompt for AI Agents
In `@public/app/features/alerting/unified/hooks/useAbilities.ts` around lines 287
- 334, The hook useAllGrafanaPromRuleAbilities must return NotSupported for all
actions when rule is undefined (skipToken); inside the useMemo in
useAllGrafanaPromRuleAbilities, add an early guard like if (!rule) { return an
Abilities<AlertRuleAction> object where every AlertRuleAction maps to
NotSupported } so
MaybeSupported/MaybeSupportedUnlessImmutable/duplicateSupported are not allowed
to default to AlwaysSupported and accidentally enable actions for non-Grafana
rules; update the returned abilities to use NotSupported for Duplicate, View,
Update, Delete, Explore, Silence, ModifyExport, Pause, Restore,
DeletePermanently when rule is missing.
Comment on lines
+157
to
163
| <GrafanaRuleListItem | ||
| rule={rule} | ||
| groupIdentifier={groupIdentifier} | ||
| namespaceName={ruleWithOrigin.namespaceName} | ||
| showLocation={true} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "FilterView.tsx" -path "*/alerting/unified/rule-list/*" 2>/dev/null | head -5Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 68
🏁 Script executed:
cat -n "public/app/features/alerting/unified/rule-list/FilterView.tsx" | sed -n '140,175p'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1689
Add a stable key for GrafanaRuleListItem list items.
GrafanaRuleListItem is rendered inside a .map() loop (line 151) and lacks a key prop, while the other components in the same switch statement (DataSourceRuleLoader on line 165 and UnknownRuleListItem on line 169) include key={key}. The key variable is available in scope from the destructuring on line 152.
✅ Suggested fix
return (
<GrafanaRuleListItem
+ key={key}
rule={rule}
groupIdentifier={groupIdentifier}
namespaceName={ruleWithOrigin.namespaceName}
showLocation={true}
/>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <GrafanaRuleListItem | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> | |
| ); | |
| <GrafanaRuleListItem | |
| key={key} | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> | |
| ); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 157-163: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🤖 Prompt for AI Agents
In `@public/app/features/alerting/unified/rule-list/FilterView.tsx` around lines
157 - 163, The GrafanaRuleListItem rendered inside the .map() loop is missing a
stable key prop which can cause React list rendering issues; update the
GrafanaRuleListItem JSX to include key={key}, matching the other branches
(DataSourceRuleLoader and UnknownRuleListItem) that already use the key variable
from the map destructuring so each item has a stable identifier.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes
Refactoring